Skip to content

ADR-0002: Implement the PHI compliance data layer, enforced access logging, and authenticated compliance API - #2

Merged
nicholas-ruest merged 3 commits into
mainfrom
adr/0002-phi-compliance-data-layer
Jul 27, 2026
Merged

ADR-0002: Implement the PHI compliance data layer, enforced access logging, and authenticated compliance API#2
nicholas-ruest merged 3 commits into
mainfrom
adr/0002-phi-compliance-data-layer

Conversation

@nicholas-ruest

Copy link
Copy Markdown
Member

Implements docs/adr/ADR-0002-implement-phi-compliance-data-layer.md, which makes the concrete technical decisions for ADR-0001 Phases 2-3. The ADR is included here since it was not previously committed. Stacked on #1 conceptually but independent in code — #1 is documentation only and touches no services/ file.

Migration execution status — read this first

The migration WAS executed against a real PostgreSQL 16 database, not merely written and reviewed.

I ran postgres:16-alpine in a local Docker container, applied V001_initial_schema.sql, then applied V002_compliance_schema.sql on top. It applied with zero errors, and re-applying it is clean (idempotent). All 15 tables verified present via information_schema. The 13 integration tests below execute real SQL against that database through the actual service code.

What that does NOT cover, and you should not read it as covering: this was an ephemeral local container, not a staging or production database. It had no pre-existing data, no app_user role (so the REVOKE block was skipped by its IF EXISTS guard and is untested), and no replication or connection-pooler in front of it. No live infrastructure was touched. A real deploy still needs a rehearsal against a database that has the application role provisioned.

Why

migrations/V001_initial_schema.sql is the only migration in the repo and defines nine tables. The compliance services reference fifteen others, none of which exist anywhere in any .sql file. Every endpoint on all three routers fails at its first query with relation ... does not exist. The service has never successfully served a request against a real database. On top of that, all three routers were mounted behind executionContextMiddleware — span plumbing — so they were reachable with no authentication at all, and GET /phi-access would have returned patient identifiers and IP addresses to anyone who could reach port 3009.

I verified each of the ADR's six findings against the code before changing anything. All six reproduce exactly as described, including the line numbers.

What changed

1. migrations/V002_compliance_schema.sql (new) — all fifteen tables

phi_access_logs is partitioned by RANGE (timestamp) mirroring the audit_logs pattern, with a DEFAULT partition so an insert never fails on a missing range — for an audit control, rejecting the write is the wrong failure direction. Indexed on (patient_id, timestamp DESC), (user_id, timestamp DESC), and (timestamp DESC): the three filters at hipaaService.ts:116-136. patient_id is CHAR(64) since the HMAC output is fixed-width, and key_version is present from day one so a future key-rotation ADR isn't blocked on a migration.

Append-only is enforced by a BEFORE UPDATE OR DELETE trigger that raises, plus a REVOKE — belt and braces, because a future GRANT would silently re-open mutation but cannot disable the trigger. This matters specifically for partitioned tables: REVOKE on the parent does not cover a direct mutation of a partition, whereas the row trigger propagates to all of them.

I confirmed each of the three schema gotchas the ADR documents, by executing them:

Gotcha Why Verified
business_associate_agreements.documents JSONB NOT NULL DEFAULT '[]' hipaaService.ts:360 appends with ||, which returns NULL on a NULL left operand and would erase the list Two successive appends → jsonb_array_length = 2
hipaa_breaches.phi_types / .containment_actions TEXT[] :719 passes JS arrays as parameters directly, without JSON.stringify Round-trips ['name','ssn','diagnosis'] intact
compliance_controls needs surrogate id and distinct control_id :93 looks up by id; hipaaService.ts:613-616 joins on control_id against identifiers like 164.308(a)(1) Both queries work against the same row

One thing the ADR did not flag, which I found and did not silently paper over: compliance_audits.findings is written two incompatible ways by existing code. The INSERT at complianceService.ts:296 passes JSON.stringify(audit.findings) (implying JSONB), but the UPDATE at :439 uses array_append(findings, $1), which requires a real array type and fails against JSONB. I made the column TEXT[], because array_append constrains the type and AuditSchema.findings is z.array(z.string()). This means the INSERT at :296 will fail'[]' is not a valid text[] literal. I did not change complianceService.ts to match, because that is a service-logic fix outside this ADR's stated scope and I would rather you see the inconsistency than have me pick a side quietly. It needs a one-line follow-up changing JSON.stringify(audit.findings) to audit.findings.

2. Authentication on all three routers

Promoted billing's middleware into services/compliance/src/middleware/auth.ts along with the minimal utils/errors and utils/logger it needs.

The field-name trap is fixed. Billing sets req.user.userId; the compliance routes read req.user?.id, which AuthenticatedUser does not have. Mounting the middleware unchanged would have looked like a fix while still attributing every record to 'system'. I found 9 such sites, not the 2 the ADR cites — the same bug is in routes/compliance.ts (5) and routes/dataResidency.ts (4). All nine now read req.user!.userId, and the || 'system' fallback is deleted everywhere; a test asserts the string is gone from all three routers.

Mount order is authenticateexecutionContextMiddleware, deliberately. The latter rejects requests missing X-Parent-Span-Id with 400, so the documented order would have made unauthenticated requests return 400 instead of 401, and would do span work for an unauthenticated caller.

Per-route guards: GET /phi-access and POST /phi-access/report require compliance-auditor/admin; BAA and breach writes require compliance-officer/admin; /requirements and /assessment require any authenticated user. Zod schemas (already a declared, unused dependency) validate every body and query, and are .strict() — so a forged userId in the body is a 400, not a silently ignored field.

3. pseudonymize() → keyed HMAC-SHA256

Deterministic, non-reversible, fixed 64-char output. No other call site needed changing; :65 and :123 simply start agreeing with each other.

4. Fail-closed key loading

config/env.ts throws when NODE_ENV=production and HIPAA_ENCRYPTION_KEY or HIPAA_PSEUDONYM_SALT is unset. Both string literals deleted. I extended this to JWT_SECRET, which the ADR does not name: billing defaults it to 'development-secret', and a default JWT secret on a PHI API means anyone can mint a valid token for any user and any role. Failing closed was the more conservative choice. Outside production an explicitly-labelled ephemeral key is permitted and logs a warning that pseudonyms are not stable across restarts.

5. PHI data-access layer

PHIRepository is now the only module permitted to issue SQL against a PHI-classified table. It emits the access record as a side effect of the operation, computes the tamper-evidence hash chain, and takes userId from an injected context — PHIAccessRecord deliberately has no userId field to forge. Reading the disclosure log is itself logged as a disclosure.

Test output — real, not fabricated

npx jest against a freshly created PostgreSQL 16 database with both migrations applied:

Test Suites: 7 passed, 7 total
Tests:       59 passed, 59 total

Every ADR Verification item, mapped:

# Verification Suite Result
1 Pseudonym determinism, incl. separate process pseudonymize.test.ts 7 pass
2 Round-trip patient lookup integration.db.test.ts pass
3 uniquePatients:1, uniqueUsers:4 from 20 accesses integration.db.test.ts pass
4 401 on every route, by enumerating the router stack auth.test.ts pass
5 403 without compliance-auditor auth.test.ts pass
6 Forged body userId does not override token auth.test.ts pass
7 No 'system' attribution leaks auth.test.ts pass
8 Fail-closed startup; literal absent from services/ failClosed.test.ts 8 pass
9 Append-only UPDATE/DELETE both raise integration.db.test.ts pass
10 Migration integrity + schema/call-site drift check schema-drift.test.ts 4 pass
11 Logging cannot be bypassed phi-enforcement.test.ts 5 pass

Selected real output:

  Verification 4: unauthenticated rejection
    ✓ returns 401 for EVERY /api/v1 route with no Authorization header (1100 ms)
    ✓ leaves /health reachable without auth (115 ms)
  Verification 6: attribution is not forgeable
    ✓ ignores a userId in the request body and attributes to the token subject (43 ms)
    ✓ does not let the internal key reach any route other than PHI ingestion (43 ms)
  Verification 7: no 'system' attribution leaks
    ✓ persists createdBy equal to the token subject on POST /hipaa/baa (29 ms)
    ✓ has no `|| 'system'` attribution fallback left in any compliance route (9 ms)
  PostgreSQL integration
    ✓ round-trips a patient-scoped PHI access record (Verification 2) (109 ms)
    ✓ reports uniquePatients:1 and uniqueUsers:4 for 20 accesses by 4 users (420 ms)
    ✓ rejects UPDATE on phi_access_logs (Verification 9) (105 ms)
    ✓ rejects DELETE on phi_access_logs (Verification 9) (95 ms)
    ✓ DETECTS a forged entry whose hash does not match its contents (37 ms)
  pseudonymize()
    ✓ returns the same value from a SEPARATE PROCESS with the same configured key (21463 ms)

Four tests are deliberately negative, because a check that only ever passes is indistinguishable from a check that is broken. Each writes a real defect, asserts the detector catches it, then removes it: an ePHI read that bypasses the repository; a query against a nonexistent table; a forged hash-chain entry; a fabricated SOC 2 Type II certified claim (that last one in #1).

The hash-chain verifier caught a genuine planted row on its first run — a hand-crafted entry left over from my schema smoke test — before I had written the test for that case. That is why the tamper-detection test exists.

Typecheck: npx tsc --noEmit reports the same 4 errors before and after my change. All four are pre-existing, from the cross-service ../../ai-platform/src/middleware/executionContext import at index.ts:19 violating the service's own rootDir. I verified this by stashing my work and running against main: byte-identical output. My changes introduce zero new type errors, and I did not fix the pre-existing four — that's a tsconfig/monorepo question, not this ADR's.

Deviations from the ADR — please review these four

1. Auth middleware is a copy, not a shared package (ADR step 5). The ADR says promote it to a location both services import, and warns "Do not fork it; a second copy of authentication logic is how the two drift." It also permits services/compliance/src/middleware/auth.ts "if extraction is deferred". I took the deferred option: the repo has no npm workspaces, the root package.json contains one dependency, and each service's tsconfig sets rootDir: ./src so TypeScript refuses to compile a file outside it. Building shared-package infrastructure across eleven services is a much larger change than this ADR. Mitigation: auth-drift.test.ts fails if billing renames the userId field, if the two AuthenticatedUser shapes diverge, or if authorize()'s role-matching rule changes in one copy — i.e. it catches exactly the drift the ADR warns about.

2. authenticateInternal intentionally diverges from billing's. Billing's synthesises userId: 'system'. Compliance's must not: phi_access_logs.user_id is UUID NOT NULL REFERENCES users(id), so an audit record must name a real person, and 'system' is not a UUID. Internal ingestion instead supplies an explicit onBehalfOf UUID and is rejected without one. This is the more conservative reading — an unattributable PHI record is worse than a rejected request — but it is a design decision the ADR left open, so flagging it. Note the residual property: anyone holding INTERNAL_SERVICE_KEY can still attribute an access to any user. That is inherent to service-to-service ingestion as the ADR specified it. Tests confirm the key reaches only POST /phi-access and no other route.

3. .env.example was NOT updated (ADR step 4). My sandbox denies read and write on .env* files, so I could not add the variables. Someone needs to add these by hand:

# Compliance service (services/compliance) - required in production; the service
# refuses to start without them. See docs/adr/ADR-0002-implement-phi-compliance-data-layer.md
HIPAA_ENCRYPTION_KEY=
HIPAA_PSEUDONYM_SALT=
HIPAA_KEY_VERSION=1
JWT_SECRET=
INTERNAL_SERVICE_KEY=

4. Deployment manifests were NOT added (ADR step 8 / ADR-0001 step 16). This needs a human decision, so I stopped rather than guessing. No service in this repo has a Dockerfile, and none of the eleven Node services appear in docker-compose.yml — only copilot-agent, postgres, redis, nats, and observability. Adding compliance alone would require inventing a Dockerfile and a build pipeline, and choosing a secret store for the keys above. Those are deployment-architecture decisions I should not make unilaterally in a PHI-handling PR. Consequence: none of this is exercised in CI yet. The tests run locally and need a Postgres service container wired into the workflow (I could not touch .github/workflows/ — the token lacks workflow scope, same as #1).

Left undone

  • The compliance_audits.findings INSERT/UPDATE inconsistency described above — one line in complianceService.ts, deliberately left for a decision.
  • REVOKE UPDATE, DELETE ON phi_access_logs FROM app_user is guarded by IF EXISTS and was not exercised, since the test container has no app_user role. The trigger — the real enforcement — is fully tested.
  • The ePHI inventory the ADR flags as a prerequisite still does not exist. PHI_CLASSIFIED_TABLES currently registers only phi_access_logs, which is correct today (no other repo code reads ePHI), but it is a registry to grow, not a finished inventory. Adding a table to it immediately brings it under enforcement.
  • Key rotation: key_version is stored but no rotation procedure exists. The ADR defers this to a follow-up, and the column being present from day one means that ADR won't need a migration.
  • ADR-0001 Phase 4 (third-party risk analysis, BAA template, CPA engagement) is unchanged and still blocks any "HIPAA-compliant" claim. This PR does not unblock that gate.

…ted API

The compliance service queried fifteen tables that no migration defined, so every
endpoint on all three routers failed at its first query. It was also reachable
with no authentication at all.

Schema: adds migrations/V002_compliance_schema.sql creating all fifteen tables.
phi_access_logs is partitioned by timestamp with a DEFAULT partition, and is
append-only -- UPDATE and DELETE raise from a trigger, with a REVOKE as defence
in depth -- plus a prev_hash/entry_hash chain for tamper evidence.

Auth: promotes billing's JWT middleware into the compliance service and mounts it
on all three routers, ahead of executionContextMiddleware so an unauthenticated
request gets 401 rather than 400. Fixes the field-name trap that would have made
this a silent no-op: the routes read req.user.id, which AuthenticatedUser does not
have, so every record was attributed to the literal 'system'. All nine sites now
read req.user.userId, and the || 'system' fallback is gone. GET /phi-access
requires compliance-auditor; BAA and breach writes require compliance-officer.
Adds Zod validation, which makes a forged userId in the body a 400.

Pseudonymization: pseudonymize() becomes keyed HMAC-SHA256 -- deterministic,
non-reversible, fixed-width. The previous random-IV AES meant patient-scoped
lookup never matched and uniquePatients counted one patient per access, making
disclosure accounting under 164.528 impossible.

Key material: removes the hardcoded default key and salt. The service now refuses
to start when NODE_ENV=production and HIPAA_ENCRYPTION_KEY, HIPAA_PSEUDONYM_SALT,
or JWT_SECRET is unset.

Enforced logging: PHI access now routes through PHIRepository, which emits the
access record as a side effect of the operation rather than trusting callers to
report themselves. Reading the disclosure log is itself logged.

59 tests across 7 suites, run against PostgreSQL 16 with both migrations applied.

Implements copilot-agent/docs/adr/ADR-0002-implement-phi-compliance-data-layer.md
…E role

Adds .github/workflows/compliance-service.yml: a Node job with a PostgreSQL 16
service container that applies V001, applies V002, re-applies V002 to prove
idempotency, asserts all fifteen tables exist, then runs the suite. Kept out of
ci.yml because every job there is a cargo job, and so this does not conflict with
the ci.yml change in the ADR-0001 branch.

Fixes a real defect found while writing it. V002 guarded its REVOKE on a role
named app_user, but the role V001 creates is llm_copilot_app, so the guard never
matched and the REVOKE never ran. That mattered: V001:444 grants SELECT, INSERT,
UPDATE, DELETE on ALL TABLES to that role, so the application role inherited
UPDATE and DELETE on the PHI audit log. Verified against a real database that the
role now holds SELECT and INSERT but not UPDATE or DELETE, while retaining UPDATE
on audit_logs as a control.

The CI database is named llm_copilot_agent because V001:442 grants on that name;
under any other name V001 fails partway and never creates the role. That is why
this was not caught earlier.

Adds an integration test asserting the privileges, closing the "REVOKE untested"
gap reported previously. Suite is now 60 tests across 7 suites.

Implements copilot-agent/docs/adr/ADR-0002-implement-phi-compliance-data-layer.md
createAudit passed JSON.stringify(audit.findings) for a TEXT[] column. Postgres
rejects the resulting '[]' with:

  ERROR:  malformed array literal: "[]"
  DETAIL:  "[" must introduce explicitly-specified array dimensions.

Reproduced against PostgreSQL 16 before fixing. node-pg serializes a JS array to
a proper array literal, so passing audit.findings directly is all that is needed.

The column must be TEXT[] rather than JSONB because the UPDATE at :439 uses
array_append, and mapAuditRow at :903 reads it back as string[] -- both of which
only work with a real array type. This INSERT was the one call site that had not
been updated to match.

Adds an integration test covering the full round trip: create an audit, append
two findings via array_append, and read them back through mapAuditRow. Verified
the test genuinely catches the defect by reverting the fix, which reproduces the
malformed array literal error.

Also makes the tamper-evidence forgery test transactional. It previously committed
a forged row to prove verifyChain detects it, but phi_access_logs is append-only so
the row could never be removed -- it broke the chain permanently and every later run
against the same database failed. The forgery is now rolled back, and verifyChain
takes an optional executor so it can inspect uncommitted rows. Confirmed by running
the suite twice against one database: 15 passed both times.

Implements copilot-agent/docs/adr/ADR-0002-implement-phi-compliance-data-layer.md
@nicholas-ruest
nicholas-ruest merged commit eefa759 into main Jul 27, 2026
5 of 21 checks passed
@nicholas-ruest
nicholas-ruest deleted the adr/0002-phi-compliance-data-layer branch July 27, 2026 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant